Pull Requestに動作確認方法を書け
まとめ
自分がどう動作確認しているかを書きましょう
コードだけでなく、動作確認方法もレビューしてもらいましょう
動作確認方法がレビューできれば、手元で動かさなくてもLGTMできる、かもしれない
pull requestのレビューというのは、責任の共有の為のダブルチェックなのだから まずチェック項目に漏れが無い事を確認するべき
設定画面を変更したけど、これログイン画面にも影響しているよね、とか
次にチェック項目を2人とも実行してみて、本当にokか確認する
悪いレビュー依頼
確認項目を明かさずレビューを依頼する
すると、両者がランダムに確認するだけになる
これはダブルチェックになっていない
両者が「相手なら自分の想定外の影響範囲もチェックしてくれてるだろ・・たぶん・・」という適当なLGTMを出す
どこを見てるのかが重要
bugを探す時に、「どこを見ていないか」で推理できる
一見ちゃんと実装したように見えても、後で必ずbugは出るから
元々責任の共有の為の風習なので、軽くやるべきなのは確かなのだが 確実に責任の負荷を分散する為に
確実にダブルチェックしましょう
動作確認方法が明示されていないと、どちらか一方に責任が偏ってしまう
互いの想定している影響範囲が噛み合わない時に、変なゆずりあいの精神が発揮される
すると
bugを見落としたり
bugが起きた後の原因究明が遅れたりする
原因
「どこを見なかったか」が共有されていない為
対策
想定している影響範囲を共有しあおう
その為に、動作確認方法を書こう
自分のOSSにプルリクが来た時の気持ちshokai.icon
99%が、「何を変更した」しか書いてない
そんなものはコード読めばわかる
なぜ変更したのか？も知りたい
できれば「その影響範囲はどこまでだと考えているのか」も知りたい
わからなければ別にいいけど
俺はもうこのライブラリの実装覚えてないんだよ
どうやってお前のプルリクが余計なバグを産み出さないか検証すればいいのかわからん
以下は思考
レビュアーに対してどこをどう操作してチェックすれば良いか具体的に書く
これはレビュアー側から見ると
pull request作成者が影響範囲がどこまでだと想定しているのかわかる
「この変更は、新規アカウント作成した所から動作確認しないとだめだよね」とか言える
コードだけでなく、確認方法についてレビューできる
レビュアーが手元で動作確認しなくても、方法だけレビューすれば良いかもしれない？
2人が動作確認した方がもちろん良いが
まあ場合による
ある程度アプリケーションの規模が大きくなってくると、ある変更がどこまでの影響範囲だと思っているのかがコミュニケーションにおいて重要
あのへん影響ありそうだけど、◯◯氏なら想定しているだろうしレビューでチェックしなくていいかな？？
とか、お互いに考えてしまう
これは面倒くさい、というかとても危険です。ヤバイ。shokai.icon*5
それを指摘するのもコメント1往復して時間が無駄
「文言の変更だけなのでCI通れば良いと思います」とだけ書いてくれるだけで、助かる
そう思っているなら、そう書く
レビュアーに推測をさせない
純粋にレビューに集中させる
逆にレビュアーは「書かれていない事についてプルリク作成者は一切考慮していない。見落としている」と考えるべき
レビュアーは「文言の変更の影響範囲が意外な所まで及んでいないか」がすごく心配
長くなったテキストがデザイン破壊しないか？
英語版の言語ファイルも同時に修正しなければならない？
この文言が表示される状況は、どのタイミングなんだろう？
とか
確認操作を書く事で影響範囲が明確になる
レビューがしやすくなる、だけではない
プルリク作成者が、レビュー希望状態にする前に自力で気づくきっかけにもなる
あっちにも影響ありそうだぞ、とか
自分のpull requestの影響範囲がどこまでなのかを明確にし、それを動作確認手順として共有するとよい
レビュアーは、その影響範囲の境界の認識についてまずレビューする
レビュアーは、pull requestの内容を見て自分が考えだした動作確認方法が正しいのか自信が持てない
ゼロから考えるのは難しい
先にたたき台があると叩ける
「動作確認方法」が書いていない場合のレビュアーの気持ち
この変更の影響範囲をアプリケーション全体から特定するために全体のコードを読むぞ
いくらコード読んでもまだ心配だ
今日はもう寝よう
次の日「よく覚えてないけどまあいいや、LGTMです！」
まとめ
pull requestを出す側は
具体的に自分がどこをどういう手順で操作して、動作確認したか、を書く
どこをレビューでチェックしてほしいか、ではない
いやどこを見てほしいかも書いても良いんだけど、そっちじゃない
変更した場所だけでなく、影響しそうな場所もローカルで動作させてみて確認するよね、それも書こうね
という話
きっかけ
自分の所に来るOSSのpull requsetにレビュー方法とか一切書いて無くてmergeできない
もうコードが頭に入ってないので、その変更の影響範囲を想像できない
せめてpull request送る側の頭の中で想定している影響範囲を教えてほしい